Consolidate text between ARGS SPEC and UG (part 1 of 3) #604
Conversation
…task/567/models-A-D
emlys
changed the title
src/natcap/invest/carbon.py
Outdated
| "name": "Calculate Sequestration" | ||
| "Enable sequestration analysis. This requires inputs " | ||
| "of land use/land cover maps for both current and future " | ||
| "scenarios. ") + REQUIRED_IF_SELECTED % ( |
There was a problem hiding this comment.
I appreciate trying to unify the language for these common phrases. Is there a code style preference of concatenation like you have here or using f strings like above on line 39? Or, whatever looks most readable?
There was a problem hiding this comment.
I was trying to use f strings as much as possible but this one wouldn't all fit in one line
src/natcap/invest/carbon.py
Outdated
| "selected, this should be the reference, or baseline, future " | ||
| "scenario against which to compare the REDD policy scenario. " | ||
| f"{REQUIRED_IF_SELECTED % 'calculate sequestration'}"), | ||
| "name": "future land use/land cover" |
There was a problem hiding this comment.
I almost wonder if we should have a LULC constant similar to the others for land use/ land cover. I'm always writing that in different ways.
There was a problem hiding this comment.
Maybe because capitalization could make this tricky if it's going as the first thing in a sentence, we could have a document with commonly used terms to reference when writing new models. Sounds a bit tedious, but I'd love to not have to think about whether I should write: LULC, or land use/land cover or land use land cover, etc...
There was a problem hiding this comment.
I think I prefer a styleguide as the solution to this problem.
There was a problem hiding this comment.
I almost wonder if we should have a LULC constant similar to the others
I like this
One advantage of a constant over a style guide is reducing repeated text - will reduce the number of words we need to get translated.
There was a problem hiding this comment.
Thinking about this more, it probably won't help with translation. We can swap out translated text at the paragraph level, and probably sentences, but words/phrases need to be translated in context. So the benefit of using a constant is really just for consistency.
src/natcap/invest/carbon.py
Outdated
| "A GDAL-supported raster representing the land-cover of the " | ||
| "current scenario."), | ||
| "name": "Current Land Use/Land Cover" | ||
| "A map of land cover for the current scenario. " |
There was a problem hiding this comment.
Should land cover be land use/land cover as you've used consistently below? I also might just be going too far with thinking how consistent these should be...
There was a problem hiding this comment.
I'm going with LULC everywhere for now
src/natcap/invest/carbon.py
Outdated
| "lucode": { | ||
| "type": "integer", | ||
| "about": ( | ||
| "Land use/land cover code. Every value in the LULC " |
There was a problem hiding this comment.
Should we use LULC here? I think it's the first time it's been used to describe land use/land cover. Again, I'm probably taking this too far, haha
There was a problem hiding this comment.
Following the 'spell out the first time' rule in this context may not be possible because the order of displaying these strings in the UG is not guaranteed to be the same. I think for common acronyms (LULC, DEM, AOI, TFA) it should be ok to define them only once for the whole user's guide (like on the data sources page). But also, sometimes it's helpful to write out.
There was a problem hiding this comment.
At least for me, TFA is not like those others in terms of the recognizability of the acronym.
| "name": "Annual Rate of Change in Price of Carbon" | ||
| "The annual increase of the price of carbon. " | ||
| f"{REQUIRED_IF_SELECTED % 'run valuation model'}"), | ||
| "name": "annual price change" |
There was a problem hiding this comment.
I'm not sure this name or the about section get at this being a rate of change versus absolute change.
There was a problem hiding this comment.
I changed this to "The relative annual increase". Hopefully that plus the type (ratio) will make it clear enough?
src/natcap/invest/carbon.py
Outdated
| _TMP_BASE_FILES = { | ||
| 'aligned_lulc_cur_path': 'aligned_lulc_cur.tif', | ||
| 'aligned_lulc_fut_path': 'aligned_lulc_fut.tif', | ||
| 'aligned_lulc_redd_path': 'aligned_lulc_redd.tif', | ||
| } | ||
|
|
There was a problem hiding this comment.
Actually I was wrong about this, not sure why I removed it.
| }, | ||
| "do_economic_analysis": { | ||
| "name": "Calculate Net Present Value of Sequestered Carbon", | ||
| "name": "Do Valuation", |
There was a problem hiding this comment.
I think this name should more reflect the valuation being done at the cost of being a little wordy. Maybe I just don't like the Do part. Calculate Valuation, Run Valuation...
| "The price of CO2E at the baseline year. Required if Do " | ||
| "Valuation is selected and Use Price Table is not selected."), |
There was a problem hiding this comment.
Should this use the REQUIRED_IF_SELECTED?
f"{REQUIRED_IF_SELECTED % 'Do Valuation'} and Use Price Table is not selected".
There was a problem hiding this comment.
Right now the REQUIRED_IF_SELECTED is a standalone sentence ending with a period, so it wouldn't work exactly here.
tests/test_args_specs.py
Outdated
| if any(l.isupper() for l in arg['name']): | ||
| print(arg['name']) |
There was a problem hiding this comment.
yea, I was making sure to lowercase all the names. Removed this.
There was a problem hiding this comment.
Hey @emlys , this is looking great. Also a shout out to whoever is updating the Args Spec wiki portion with these changes / ideas in mind. My first thought was, we should make sure to get this down in the wiki, and it already looks like it's there.
A few things came to mind when reviewing.
- Do we have formal guidelines on
LULCvsland use/land covervsland cover? - How do others feel about the
REQUIRED_IF_SELECTEDhelper for consistent phrasing? I think I like this approach but do think the only benefit is consistency, since it doesn't really shorten anything and is more syntax onerous. - Side-Note: Should we update the
argssection of the wiki to better define what information should go in that docstring?
src/natcap/invest/carbon.py
Outdated
| "selected, this should be the reference, or baseline, future " | ||
| "scenario against which to compare the REDD policy scenario. " | ||
| f"{REQUIRED_IF_SELECTED % 'calculate sequestration'}"), | ||
| "name": "future land use/land cover" |
There was a problem hiding this comment.
I think I prefer a styleguide as the solution to this problem.
| "Annual increase in the price of CO2E. Required if Do " | ||
| "Valuation is selected and Use Price Table is not selected.") |
There was a problem hiding this comment.
I can imagine that when editing a bulk of these it came in handy. But going forward, adding one or two new specs 6 months or a year from now, I think it will be easier for me to write the whole sentence here. Also easier to verify that it's correct grammar.
I don't feel too strongly so I'm okay with leaving these in. I'm just not sure I'd prefer to use them myself in the future.
| "about": ( | ||
| "Path to a polygon vector that is projected in a coordinate " | ||
| "system with units of meters. The polygon should intersect " | ||
| "the landmass and the shelf contour line"), |
There was a problem hiding this comment.
Thanks, this bit about the shelf contour line is outdated and not true anymore.
| }, | ||
| "model_resolution": { | ||
| "type": "number", | ||
| "expression": "value > 0", | ||
| "units": u.meter, | ||
| "about": ( | ||
| "Distance at which the coastline will be resolved. Coastline " | ||
| "features smaller than this distance will not be represented " | ||
| "by the shoreline points. Points will be spaced at intervals " |
There was a problem hiding this comment.
Thanks again, this bit about small coastline features not being represented isn't accurate either, though maybe it should be.
There was a problem hiding this comment.
Oh, I just moved that phrase over to the RST side... I'll take it out completely.
| "polygon because some functions in the model require " | ||
| "searching for landmasses around shore points up to the " | ||
| "distance defined in Maximum Fetch Distance, which likely " | ||
| "extends beyond the AOI polygon."), |
| }, | ||
| "dem_path": { | ||
| "type": "raster", | ||
| **spec_utils.DEM, | ||
| "bands": {1: { | ||
| "type": "number", | ||
| "units": u.linear_unit # any unit of length is ok |
There was a problem hiding this comment.
I commented on the UG PR about omitting units: none from the rendered docs. I sort of feel similarly here, where the docs "units: linear unit" is a point of confusion at worst and just ignored by the reader at best. Honestly, I think CV would work fine even if the DEM has height units in decimal degrees, which would be very weird.
Not sure what the solution is. It's slightly inaccurate to change the spec to u.none. And I'm guessing a units attribute is required for number types? Any thoughts?
There was a problem hiding this comment.
If 'linear unit' is too confusing, I'll just go with u.none. While any unit would work, you could even have a unitless "height index" and that would still work as long as higher points have bigger values.
| "2": "low", | ||
| "3": "moderate", | ||
| "4": "high", | ||
| "5": "very high" |
There was a problem hiding this comment.
These descriptions need to be inverted actually. 5 is the rank for no habitat protection at all. 1 is for very high protection.
Or in other words, once the ranks are assigned to each shore point, 5 is very high exposure at a point, 1 is very low exposure. That matches how the other exposure variables are scaled.
But I think when preparing this CSV, it's more intuitive to think in terms of "protection offered". Can we invert them and add "protection" to each one for good measure?
There was a problem hiding this comment.
Oh okay, that makes more sense.
| }, | ||
| "about": ( | ||
| "Relative amount of coastline protection this habitat " | ||
| "provides.") |
| "3": "moderate", | ||
| "4": "high", | ||
| "5": "very high" | ||
| }, |
There was a problem hiding this comment.
This one's perfect because it's in terms of exposure. If we want to we could again add "exposure" to the end of each description.
| "Exposure rank to assign to any shore points that are not " | ||
| "near to any segment in the geomorphology vector. " | ||
| f"{REQUIRED_IF_PROVIDED % 'Geomorphology vector'}"), | ||
| "name": "geomorphology fill value" | ||
| }, | ||
| "population_raster_path": { | ||
| "type": "raster", | ||
| "bands": { | ||
| 1: {"type": "number", "units": u.none} |
There was a problem hiding this comment.
I'm fine with leaving this as u.none and letting the about section do the describing. But if there's a u.count that would also be accurate.
There was a problem hiding this comment.
I'd prefer not to make the count vs none distinction, since I don't fully understand it
Co-authored-by: Doug <[email protected]>
Co-authored-by: Doug <[email protected]>
…into task/567/models-A-D
|
As we agreed in the coffee call this morning, I took out all the f-string variables and instead we'll follow a style guide for those things. |
|
ReadTheDocs build is failing on GDAL install with this error: |
|
The setuptools issue only happened because I forgot to update the GDAL requirement in So we don't need to do anything about this, and I'm going to merge once the tests pass. Before: expected failure when the gdal library isn't already installed Now: different failure due to setuptools change Looks like they may have resolved this for gdal 3.3.1 also. |
emlys
force-pushed
the
task/567/models-A-D
branch
from
703a796
to
639b2ee
Compare
Description
First part of #567.
Checklist
- [ ] Updated HISTORY.rst (if these changes are user-facing)- [ ] Updated the user's guide (if needed)